-
Notifications
You must be signed in to change notification settings - Fork 416
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add slot methods to a module instead of the component class itself #2040
Add slot methods to a module instead of the component class itself #2040
Conversation
2430994
to
c1846e2
Compare
65e98be
to
90598cf
Compare
… `#{slot}` and `#{slot?}` methods on this module, allowing these methods to be overridden with access to `super` implementation.
90598cf
to
3a7f922
Compare
@joelhawksley after a "brief" hiatus I cam back to this and got it in working order. Unfortunately the "Test compatibility with Primer ViewComponents" is failing, and if anyone had some bandwidth to help shine some light into that failure that would be hugely appreciated. I'll try to dig in when I get another chance but I'll be wandering in a lot of unfamiliar territory as I do! It's also pretty unclear to me, at a high level, why this change would specifically break the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! I'm going to defer to @BlakeWilliams for the review here ❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for this improvement!
What are you trying to accomplish?
Addresses #2027:
super
What approach did you choose and why?
GeneratedAttributeMethods
, I am choosing the nameGeneratedSlotMethods
self::GeneratedSlotMethods
in theinherited
hook defined byViewComponent::Slotable
, similar to how ActiveRecord creates<model>::GeneratedAttributeMethods
.self::GeneratedSlotMethods
in the same inherited hook<slot>
and<slot>?
methods on the moduleTODO
inherited
hook and not theincluded
hook;ViewComponent::Slotable
is included only once byViewComponent::Base
Anything you want to highlight for special attention from reviewers?
Added a step in CONTRIBUTING tobundle install
, with the goal of installing appraisal (otherwisebundle exec appraisal install
would fail)bundle exec appraisal install
modified the gemfiles and I'm not sure why. I have no experience with appraisal. I imagine I will need to revert these changes,but I'm keeping them on the branch for now since if I revert I can't run tests, and also to ask about it.